Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

report: rename i18n to i18n-formatter, move strings to Util #13933

Merged
merged 4 commits into from
Jan 23, 2023
Merged

report: rename i18n to i18n-formatter, move strings to Util #13933

merged 4 commits into from
Jan 23, 2023

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 27, 2022 •
edited
Loading

All of renderer/i18n.js is formatting stuff, but it also doubled as a home for strings (including state, applying strings from the LHR). this pr:

  • renames report/renderer/i18n.js to report/renderer/formatter.js
  • moves the strings stuff to Util
  • new function Util.applyStrings which takes care of adding the english fallbacks

@connorjclark connorjclark requested a review from a team as a code owner April 27, 2022 21:36
@connorjclark connorjclark requested review from brendankenny and removed request for a team April 27, 2022 21:36
connorjclark
connorjclark commented Apr 27, 2022
View reviewed changes
report/renderer/util.js Outdated Show resolved Hide resolved
@connorjclark connorjclark mentioned this pull request Jan 20, 2023
Closed
adamraine
adamraine approved these changes Jan 23, 2023
View reviewed changes
core/util.cjs
@@ -805,6 +819,7 @@ const UIStrings = {
runtimeCustom: 'Custom throttling',
};
Util.UIStrings = UIStrings;
Util.strings = {...UIStrings};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason we set these "static" variables here rather than in the class declaration? Might be a good comment to add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, it's because moving UIStrings declaration to be before the class will be bad for git blame history. I'm not sure that's a particularly useful thing to call out in a comment tho

flow-report/src/i18n/i18n.tsx
return i18n;
return {
formatter: Util.formatter,
strings: Util.strings as typeof UIStrings & typeof Util.UIStrings,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
strings: Util.strings as typeof UIStrings & typeof Util.UIStrings,
strings: Util.strings as (typeof UIStrings & typeof Util.UIStrings),

@brendankenny
Copy link
Member

brendankenny commented Jan 23, 2023 •
edited
Loading

bumping strings up onto Util seems like a good move (and love dropping the type parameterization), but (bikeshed time) could i18n still be called i18n? formatter is kind of a mouthful and especially in the context of html generation makes me think of style formatting rather than localization.

@connorjclark
Copy link
Collaborator Author

bumping strings up onto Util seems like a good move (and love dropping the type parameterization), but (bikeshed time) could i18n still be called i18n? formatter is kind of a mouthful and especially in the context of html generation makes me think of style formatting rather than localization.

the main idea was to disambiguate these files:

image

I guess I'm fine with the property still being i18n, but are you also suggesting that formatter is a bad change for the file name too?

@brendankenny
Copy link
Member

Descriptive but not optimal in this context, maybe? FWIW there's also shared/localization/format.js for further name confusion, which maybe should have been called formatter.js depending on where you fall on the nouns vs verbs thing, but that really only got a name at all because core/lib/i18n/i18n.js had to be split to move some of it to shared/.

@connorjclark connorjclark changed the title report: rename report i18n to formatter, move strings to Util report: rename i18n to i18n-formatter, move strings to Util Jan 23, 2023
@connorjclark connorjclark merged commit 5e17931 into main Jan 23, 2023
@connorjclark connorjclark deleted the mv-i18n-formatter branch January 23, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamraine adamraine adamraine approved these changes

@brendankenny brendankenny Awaiting requested review from brendankenny brendankenny is a code owner automatically assigned from GoogleChrome/lighthouse-hackers

Assignees

@brendankenny brendankenny

Labels
waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@connorjclark @brendankenny @adamraine @devtools-bot